Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

BUG: fin_flutter_analysis doesn't find any fin set #510

Merged
merged 4 commits into from
Dec 20, 2023
Merged

BUG: fin_flutter_analysis doesn't find any fin set #510

merged 4 commits into from
Dec 20, 2023

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Dec 14, 2023 •
edited
Loading

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

This bug was reported through discord yesterday:
image

Also, the tests for the fin_flutter_analysis were deactivated since they were not passing.

New behavior

  • Solved the bug
  • Refactored the code to improve readability.
  • Improved the tests for flutter analysis.

Breaking change

  • No

Additional information

This feature works only for trapezoidal fins, that's said.
Also, we should improve the documentation in the future, try to give some more examples.

@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner December 14, 2023 07:46
@Gui-FernandesBR
Copy link
Member Author

I am still working on unit tests to prevent from future occasions. Please don't review it yet

@codecov Codecov
Copy link

codecov bot commented Dec 14, 2023 •
edited
Loading

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f1367e3) 71.30% compared to head (5390478) 72.02%.
Report is 2 commits behind head on develop.

❗ Current head 5390478 differs from pull request most recent head 6f4c3c3. Consider uploading reports for the commit 6f4c3c3 to get more accurate results

Files Patch % Lines
rocketpy/utilities.py 88.23% 2 Missing ⚠️
Additional details and impacted files 
@@             Coverage Diff             @@
##           develop     #510      +/-   ##
===========================================
+ Coverage    71.30%   72.02%   +0.72%     
===========================================
  Files           56       56              
  Lines         9374     9370       -4     
===========================================
+ Hits          6684     6749      +65     
+ Misses        2690     2621      -69
Flag Coverage Δ
unittests 72.02% <88.23%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Dec 14, 2023
@Gui-FernandesBR Gui-FernandesBR added Bug Something isn't working Tests Regarding Tests Refactor labels Dec 14, 2023
Gui-FernandesBR
Gui-FernandesBR commented Dec 15, 2023
View reviewed changes
rocketpy/utilities.py

# First, we need identify if there is at least a fin set in the rocket
for aero_surface in flight.rocket.aerodynamic_surfaces:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make reviewer's life easier: this was the problematic code line. The aerodynamic_surfaces is no longer a list of surfaces, instead it is a list of components;

phmbressan
phmbressan approved these changes Dec 19, 2023
View reviewed changes
Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to see these tests reintroduced, let's hope they don't give the same headaches as before.

rocketpy/utilities.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR changed the title BUG: Refactor fin_flutter_analysis function to handle multiple fin sets BUG: fin_flutter_analysis doesn't find any fin set Dec 20, 2023
@Gui-FernandesBR Gui-FernandesBR merged commit 086a924 into develop Dec 20, 2023
10 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the bug/fix-fin-flutter branch December 20, 2023 01:02
@Gui-FernandesBR Gui-FernandesBR linked an issue Dec 25, 2023 that may be closed by this pull request
TST: FinFlutterAnalysis not working properly? #367
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phmbressan phmbressan phmbressan approved these changes

@giovaniceotto giovaniceotto Awaiting requested review from giovaniceotto

@MateusStano MateusStano Awaiting requested review from MateusStano

Assignees

@Gui-FernandesBR Gui-FernandesBR

Labels
Bug Something isn't working Refactor Tests Regarding Tests
Projects
LibDev Roadmap
Status: Closed
Milestone
Release v1.X.0
Development

Successfully merging this pull request may close these issues.

TST: FinFlutterAnalysis not working properly?
2 participants
@Gui-FernandesBR @phmbressan